fix: Record handled report info during initial write #322
Conversation
kattrali
force-pushed
the
kattrali/serialize-handled-info-directly
branch
from
6354abb
to
995c13e
Compare
There was a problem hiding this comment.
The general approach of removing mutable shared state is good, and after manually testing a handled/unhandled exception, the implementation appears to work.
I think we need to add test coverage for the original scenario before releasing, as that will help verify that we've solved the original problem. If automated tests are impractical, then we need to perform integration testing via some other method, such as via manual testing.
| appState:[self.state toDictionary] | ||
| callbackOverrides:report.overrides | ||
| metadata:[report.metaData copy] | ||
| config:[self.configuration.config toDictionary] |
There was a problem hiding this comment.
Access to self.configuration.config was previously synchronized behind a lock - is this access also synchronized?
There was a problem hiding this comment.
Previously, these values were mutated in this block. Removing the mutation removed the need for a lock.
Before this change, information which is not part of the standard KS report was serialized in the onCrash callback, which is shared between notify() and fatal reports, and used simultaneously in the case that two reports are generated at once. This change serializes information which applies to only a single report into the initial report, reserving the onCrash handler for shared global state which does not become null once set, like session information, and handlers which are only run in the event of a crash (by definition, occurring only once). Fixes a case where handled state can be reported incorrect for notify() requests as the shared handled state was cleared/set to null by a concurrent request.
kattrali
force-pushed
the
kattrali/serialize-handled-info-directly
branch
from
995c13e
to
733efbe
Compare
Before this change, information which is not part of the standard KS
report was serialized in the onCrash callback, which is shared between
notify() and fatal reports, and used simultaneously in the case that
two reports are generated at once.
This change serializes information which applies to only a single report
into the initial report, reserving the onCrash handler for shared global
state which does not become null once set, like session information, and
handlers which are only run in the event of a crash (by definition,
occurring only once).
Fixes a case where handled state can be reported incorrect for notify()
requests as the shared handled state was cleared/set to null by a
concurrent request.
Tests
The existing tests cover report info serialization and I broke each component at least once. The case which this changeset fixes its hard to boil down to a test case as its the result of a timing issue.
Review
Outstanding Questions